-
Notifications
You must be signed in to change notification settings - Fork 14
feat: Add redirected actor logs #403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Update test to be able to use caplog
|
|
Refactor tests and code to reuse code.
|
Original log colors are now respected: Thanks a lot @barjin for letting me know about the secret parameter of the log endpoint! |
Update default logger to not duplicate handlers if already exists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new log redirection feature for Actor runs, allowing logs to be streamed and redirected to either a default or a custom logger. Key changes include:
- New synchronous and asynchronous methods (get_streamed_log) in the Run and Log clients for redirecting logs.
- Implementation of StreamedLog classes (sync and async) to handle chunked log messages and ensure proper formatting.
- Updates to the Actor client to support an optional logger parameter for redirecting logs, along with additional tests and dependency updates.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_logging.py | Added tests to verify correct log formatting and redirection behavior. |
| src/apify_client/clients/resource_clients/run.py | Introduced get_streamed_log methods for both sync and async clients. |
| src/apify_client/clients/resource_clients/log.py | Updated log retrieval methods with a new raw option and added StreamedLog classes for log streaming. |
| src/apify_client/clients/resource_clients/actor.py | Extended Actor.call method to support log redirection via an optional logger parameter. |
| src/apify_client/_logging.py | Added create_redirect_logger and improved RedirectLogFormatter with color support. |
| pyproject.toml | Added necessary dependencies for color support via colorama. |
Have explicit start and stop instead of __call__
32def80 to
74595f9
Compare
|
No need to do deep review @MQ37. I think you might be using this functionality soon. Could you please take a look at the use-cases and see if it is convenient to use from your point of view and if there is missing something? |
janbuchar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't managed to read the whole thing yet, but here are some comments
| logger: Logger used to redirect logs from the Actor run. By default, it is set to "default" which means that | ||
| the default logger will be created and used. Setting `None` will disable any log propagation. Passing | ||
| custom logger will redirect logs to the provided logger. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, it is set to "default" which means that the default logger will be created and used
Um, and what does that actually mean for the caller? Let's try and come up with a better description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to rephrase it like this:
.... Using "default" literal means that a predefined default logger will be used. ...
| **self._sub_resource_init_options(resource_path='log'), | ||
| ) | ||
|
|
||
| async def get_streamed_log( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused with the intended usage here. Are you supposed to call this method and then pass the result into an actor.call(logger=...) call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, actor.call(logger=...) is one group of use-cases that deal with you starting the other actor and then get_streamed_log is called internally.
Using get_streamed_log method directly has different use-case. It is when you want to stream log from already started actor (for example long-running actor in standby)
async with await Actor.new_client().run("some_id").get_streamed_log(from_start=False):
# Do stuff while logs from other actor are redirceted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checked the example usage and from UX viewpoint this looks good, thank you 👍 @Pijukatel
df4afce to
cba571f
Compare
vdusek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the Actor call log propagation work recursively? (Actor calls an Actor who calls another Actor...)
Update test data to properly match the reality with line endings
Naming.
c29b896 to
669a749
Compare
| mock_api_sync: None, # noqa: ARG001, fixture | ||
| propagate_stream_logs: None, # noqa: ARG001, fixture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do it better here? Maybe pytest-mark-usefixtures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janbuchar I could not find the comment, but I remember we were already discussing it some time ago in another review and you were in favor of # noqa.
I have no problem using this fixture and I am also OK with the # noqa, so if you agree with @vdusek on the preferred way in our code base, I will just use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm mostly OK with both, but usefixtures does have the drawback of not being applicable to other fixtures...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright, so it's up to you Pepa 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no change then :-)
vdusek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
### Description - Add convenience methods and arguments for redirecting the streamed actor log to another log. - This changes the default behavior in non non-breaking way. By default, the logs will be redirected now. - Update log endpoints calls to accept `raw` parameter. - Parity with Python implementation apify/apify-client-python#403 ### Example usage: #### Actor.call - default This will redirect logs by default. Example default redirected line: > 2025-10-22T13:06:39.476Z redirect-log-tester runId:SoK1VRJxG61tVrgNz -> 2025-10-22T13:06:06.686Z ACTOR: Pulling container image of build Gh9yAJtjw0rHpc1NU from registry. ```javascript ... await client.actor('redirect-actor-id').call(); ``` #### Actor.call - custom Log This will redirect logs using custom log and logger. Example default redirected line: > 2025-10-22T13:06:39.476Z customPrefix 2025-10-22T13:06:06.686Z ACTOR: Pulling container image of build Gh9yAJtjw0rHpc1NU from registry. ```javascript ... await client.actor('redirect-actor-id').call(someInputs, { log: new Log({ level: LEVELS.DEBUG, prefix: 'customPrefix', logger: new LoggerActorRedirect() }), ``` #### Actor.call - no log redirection This will disable all log redirection (same as current behavior) ```javascript ... await client.actor('redirect-actor-id').call(someInputs, {log: null}), ``` #### Actor.run - attaching to already running actor and redirecting new logs A typical use case is redirecting logs from an Actor that runs in standby. We do not want all the logs; we only want the new logs generated from the moment of connection. ```javascript ... const streamedLog = await client.run('someActorRunId').getStreamedLog({ fromStart: false }); streamedLog.start(); // Do some stuff while also redirecting logs from another Actor await streamedLog.stop(); ``` #### Example actor with recursive redirection: https://console.apify.com/actors/IcfIKTIvbmujMmovj/source ### Issues - Partially implements: #632




Description
rawparameter.Example usage:
Starting new actor run through call
3 different use-cases:
Attaching to already running actor, redirecting all logs from actor start
(This is the technique to be used when starting actor through
actor.startas well)Attaching to already running actor, redirecting only new logs
(This can be useful when communicating with some long-running standby actor)
Example actor with all the logging options shown:
https://console.apify.com/actors/m7ZFYX42SCRFOL0JW
Issues